Skip to content

fix(config): unpin installs frozen on the retired beta-api host - #65

Open
tonychang04 wants to merge 3 commits into
mainfrom
fix/retired-api-url-migration
Open

fix(config): unpin installs frozen on the retired beta-api host#65
tonychang04 wants to merge 3 commits into
mainfrom
fix/retired-api-url-migration

Conversation

@tonychang04

@tonychang04 tonychang04 commented Jul 27, 2026

Copy link
Copy Markdown
Member

The bug

readGlobal materialises DEFAULT_API into the object it returns, and persist() writes that
whole object back after login. So the default in force at a user's first login becomes a
literal in ~/.insta/config.json and stays there forever:

readGlobal()     -> { ...parsed, apiUrl: envApi ?? parsed.apiUrl ?? DEFAULT_API }
ApiClient.load() -> new ApiClient(await readGlobal())
persist()        -> writeGlobal(this.cfg)   // cfg.apiUrl already resolved

There is no migration anywhere in the tree, and login/loginOauth only call setApiUrl when
--api-url is passed — so re-logging in does not repoint an existing install either.

DEFAULT_API was https://beta-api.insta.insforge.dev in v0.0.3 (2026-07-03) through v0.0.16
(2026-07-15 14:13Z)
; v0.0.17 moved it to https://api.instacloud.com. That host went
NXDOMAIN on 2026-07-27 when the insta.insforge.dev zone was emptied, so everyone who first
logged in during that window is hard-broken right now, with no escape but INSTA_API_URL or
hand-editing the file.

Two things make it stay invisible: autoUpdate has been on by default since 2026-07-04, so those
installs keep upgrading the binary while staying pinned; and fresh installs have been correct all
along, so anyone checking on a clean machine sees it working.

The fix

Treat a retired host as absent on read, so it falls through to DEFAULT_API.

Resolving on read (rather than rewriting the file inside a read function) fixes every invocation
immediately, and the file heals itself the next time any command persists — ApiClient.persist()
or insta upgrade --auto. readGlobal is the single choke point: ApiClient.load() is the only
consumer of apiUrl, so nothing bypasses it.

Deliberately not touched: a persisted localhost or self-hosted URL still wins — those are
real choices, and the very first release (v0.0.0–v0.0.2) defaulted to http://localhost:8080,
which we must not rewrite. INSTA_API_URL still beats everything, including the retired host;
explicit beats ambient.

Scoped to the exact string the CLI ever wrote (https://…, trailing slashes tolerated). An
http:// variant hand-typed via --api-url is left alone as a deliberate value.

Tests

test/config-api-url.test.ts, 7 cases — the retirement path, and the guardrails that must not
change. Verified they can actually fail: with src/config.ts reverted, 4 fail and 3 pass
the 3 that pass are exactly the must-not-change cases (localhost, INSTA_API_URL, no config file).

npx tsc --noEmit clean; full suite 20 files / 121 tests green.

Blast radius

One function, read path only. Worst case for a user not in the affected cohort is nil — their
value is untouched. No command or flag surface changes, so no skills/insta/cli-reference.md
update needed.


Summary by cubic

Treat the retired beta API host as absent when reading global config so pinned installs fall back to https://api.instacloud.com, and drop the old session to prevent cross-deployment token refresh. Prints a one-line stderr notice suggesting re-login; stdout stays clean for --json.

  • Bug Fixes
    • In readGlobal, ignore https://beta-api.insta.insforge.dev (trailing slashes ok) and use the default.
    • When replacing the host, clear accessToken/refreshToken/user and print one stderr notice to run insta login (stdout untouched). This avoids sending a retired deployment’s refresh token to a different one via the 401 refresh path.
    • Preserve explicit choices: INSTA_API_URL still wins; localhost/self-hosted URLs remain unchanged.
    • Read-path only; the file heals on the next persist.
    • Added test/config-api-url.test.ts to cover host retirement, session drop, stderr notice, and guardrails.

Written for commit 5b2b6b1. Summary will update on new commits.

Review in cubic

Release note

After this ships, jq .apiUrl ~/.insta/config.json is no longer a valid way to check whether
a user is still pinned.
Resolve-on-read means the file keeps the retired host until something
persists (login, logout, upgrade --auto on|off, or a successful token refresh — which a
stale token never reaches). A fully-fixed user who only runs read commands still has the old
value on disk.

The check that reflects reality:

insta status --json | jq .apiUrl

Credit: @qa-bot found this in review.

readGlobal materialises DEFAULT_API into the object persist() writes back, so
whatever the default was at a user's first login is frozen into config.json
permanently. beta-api.insta.insforge.dev was the default in v0.0.3..v0.0.16 and
went NXDOMAIN on 2026-07-27, leaving those installs hard-broken with no escape
except INSTA_API_URL or hand-editing the file — upgrading the binary cannot fix
them, and every fresh install looks fine.

Treat a retired host as absent on read so it falls through to DEFAULT_API.
Resolving on read fixes every invocation immediately; the file heals itself the
next time any command persists. A persisted localhost or self-hosted URL is a
deliberate choice and still wins, as does INSTA_API_URL.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Jul 27, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: fix(config): unpin installs frozen on the retired beta-api host

Summary: A tight, well-tested read-path fix that treats the retired beta-api.insta.insforge.dev host as absent so frozen configs fall through to DEFAULT_API and self-heal on next persist — clean to merge.

Requirements context

No matching spec/plan found — this repo has no docs/superpowers/ or docs/specs/ directory (only README.md, AGENTS.md, CLAUDE.md, and the developing-insta-cli skill). Assessed against the PR description, AGENTS.md conventions, and the surrounding code. Repo gate per AGENTS.md is npm run typecheck && npm test; I ran both — see Verdict.

Findings

Critical

(none)

Suggestion

  • Functionality — completeness of the retirement set (src/config.ts:32). The fix is only as correct as RETIRED_API_URLS being the complete list of hosts the CLI ever shipped as a default and later retired. The PR establishes the history (localhost in v0.0.0–v0.0.2 — deliberately excluded; beta-api… in v0.0.3–v0.0.16; api.instacloud.com from v0.0.17). That reasoning is sound and matches the current DEFAULT_API, so no change is required now — just flagging that this Set is the load-bearing assumption, and any future default-host rotation must add the outgoing host here (with a regression test) or a new cohort silently re-freezes.

Information

  • Software engineering — incidental hardening (src/config.ts:46). The new guard typeof parsed.apiUrl === 'string' && parsed.apiUrl also rejects an empty-string apiUrl, which the previous parsed.apiUrl ?? DEFAULT_API would have kept (since ?? only catches null/undefined). A persisted "" would have made fetch(this.apiUrl + path) hit a relative URL — so this is a small correctness improvement beyond the stated scope. Worth a one-line note in the PR body; no action needed.
  • Security — no security-relevant changes. No new user input reaches SQL/shell/HTTP; the apiUrl still comes only from the config file / INSTA_API_URL / --api-url (unchanged trust boundary). The .replace(/\/+$/, '') normalization regex is anchored and linear — no ReDoS. No secrets logged or returned.
  • Performance — no concerns. One Set.has lookup on a normalized string per readGlobal; no new I/O, loops, or allocations in any hot path.

Notes on what I verified

  • readGlobal really is the single choke point. Grepped every .apiUrl consumer: all flow through ApiClient (api.ts:18/62), which loads via readGlobal (api.ts:16); auth.ts only sets apiUrl explicitly on --api-url. Nothing reads the persisted value bypassing the resolver, so the fix reaches every invocation as claimed.
  • Spread order is correct (src/config.ts:47): { ...parsed, apiUrl: … } keeps accessToken/refreshToken/autoUpdate intact while overriding the stale host — covered by the "keeps the rest of the config intact" test.
  • Self-heal path is real: writeGlobal(await readGlobal()) (what ApiClient.persist() and upgrade.ts:104-106 both do) rewrites the healed value — covered by the "healed value reaches disk" test.
  • Test suite is meaningful and green: ran npx vitest run → 20 files / 121 tests pass; npx tsc --noEmit clean. The 7 new cases cover the retirement path plus the trailing-slash bypass and the three must-not-change guardrails (localhost, INSTA_API_URL, no config file), matching the PR's mutation-check claim (revert → 4 fail / 3 pass).

Verdict

approved (informational; the human still gives the explicit GitHub approval via the approve flow). Zero Critical findings — the two items above are non-blocking. Posted as a COMMENT per the verdict rule.

@jwfing jwfing left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - approved.

… backend comment

The retired host was a different deployment, not a rebrand: beta-api resolved to
insta-beta-api-lb in us-west-1, while DEFAULT_API is insta-platform-prod-alb in
us-east-2. So a session minted there is not valid at the new host, and repointing
the URL alone would have traded 'fetch failed' for an opaque 401 that reads as a
bug in this migration.

Drop accessToken/refreshToken/user alongside the host and print one stderr line
pointing at 'insta login'. stderr, not stdout, so --json output stays parseable.
The 'same backend, branded domain' comment that implied portability was wrong;
corrected in place.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tonychang04

Copy link
Copy Markdown
Member Author

Follow-up commit d2efa0d — the original diff had a wrong premise, caught in review by @qa-bot.

The retired host was a different deployment, not a rebrand. src/config.ts:22 claimed "same backend, branded domain", and I took it. It isn't true:

  • beta-api.insta.insforge.devinsta-beta-api-lb-458647037.us-west-1.elb.amazonaws.com (us-west-1)
  • api.instacloud.cominsta-platform-prod-alb-1012335656.us-east-2.elb.amazonaws.com (us-east-2)

Separate deployments in separate regions. So an accessToken minted against the retired host does not authenticate at DEFAULT_API, and repointing the URL alone would have swapped fetch failed for an opaque 401 — still broken, and it would have read as this migration's fault.

Now drops accessToken/refreshToken/user along with the host and prints one line to stderr (not stdout — printJson in commands/auth.ts:80 writes --json output there and must stay parseable):

note: https://beta-api.insta.insforge.dev has been retired; using https://api.instacloud.com. Run `insta login` to sign in again.

The false comment is corrected in place rather than left to mislead the next reader.

Tests: 3 added (session dropped, notice on stderr and not stdout, healthy config keeps its session). 124 pass, tsc --noEmit clean. Verified they can fail — reverting src/config.ts to the previous commit fails exactly the 2 new behavioural tests.

Open question that is not this PR's to answer: whether accounts and projects created against the beta deployment were carried over to prod. If they weren't, re-logging in gets these users a working CLI pointed at an account that doesn't hold their old projects. That's a data question for a human, and it doesn't block this — the current behaviour for them is a CLI that cannot reach any host at all.

Not the error message — requireProject already dies with an actionable 401 hint
and the stderr note covers the rest. The reason is that api.ts's 401 path POSTs
the stored refresh token to whatever apiUrl now resolves to, which would send a
credential minted by the retired deployment to a different one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tonychang04

Copy link
Copy Markdown
Member Author

@qa-bot reviewed 9ac0a93; head is now 5b2b6b1. Two things from that review, one conceded and one kept for a different reason than I first gave.

Conceded: my justification for clearing the session was weak. I wrote that keeping the tokens would swap fetch failed for an opaque 401. That doesn't hold up — api.ts:111 already dies with not logged in — run \insta login ...`` on the project-resolution path, and the stderr note this PR adds covers the rest. The error message is not the argument.

Kept, for the reason that does hold: it would send one deployment's credential to another. api.ts:53 retries on 401 whenever cfg.refreshToken is set, and refresh() POSTs that token to this.apiUrl — which by then is api.instacloud.com (us-east-2 prod), while the token was minted by insta-beta-api-lb (us-west-1). Every affected user's first post-upgrade command would transmit a staging-issued refresh token to prod, and it can only ever fail. Dropping the session avoids that; comment corrected in 5b2b6b1 to say so instead of the message argument.

Adopted: the post-release triage trap. Resolve-on-read leaves the dead host in the file until something persists, so jq .apiUrl ~/.insta/config.json reports "still pinned" for a user who is already fixed. The correct check after this ships is:

insta status --json | jq .apiUrl     # resolved value
insta status                         # or just read the 'api:' line

Added to the release note section below. Heal triggers confirmed as login, logout, upgrade --auto on|off, and a successful refresh — which a stale token never reaches.

Boundary noted, not changed: uppercase, http:// and :443 variants don't match the Set. No shipped DEFAULT_API could produce them, only a hand-typed --api-url, so widening the Set would add reach without a caller. Worth re-checking if it's ever extended to another host.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/config.ts">

<violation number="1" location="src/config.ts:55">
P2: With `INSTA_API_URL` set, retired configurations skip session cleanup and the recovery notice. An authenticated request that gets a 401 can then POST the retired deployment’s refresh token to the env-selected endpoint; detect/clear the retired persisted URL before applying the env API override.</violation>
</file>

<file name="test/config-api-url.test.ts">

<violation number="1" location="test/config-api-url.test.ts:58">
P3: A failed assertion in the stderr/stdout test leaves both process stream spies installed because cleanup is performed only after the assertions. Restoring the spies in a `finally` block or from `afterEach` would keep one test failure from contaminating later tests.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread src/config.ts
try {
const parsed = JSON.parse(await readFile(GLOBAL_FILE, 'utf8')) as GlobalConfig
return { ...parsed, apiUrl: envApi ?? parsed.apiUrl ?? DEFAULT_API }
if (envApi) return { ...parsed, apiUrl: envApi }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: With INSTA_API_URL set, retired configurations skip session cleanup and the recovery notice. An authenticated request that gets a 401 can then POST the retired deployment’s refresh token to the env-selected endpoint; detect/clear the retired persisted URL before applying the env API override.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/config.ts, line 55:

<comment>With `INSTA_API_URL` set, retired configurations skip session cleanup and the recovery notice. An authenticated request that gets a 401 can then POST the retired deployment’s refresh token to the env-selected endpoint; detect/clear the retired persisted URL before applying the env API override.</comment>

<file context>
@@ -19,32 +19,55 @@ export type GlobalConfig = {
-    const persisted =
-      typeof parsed.apiUrl === 'string' && parsed.apiUrl && !isRetired(parsed.apiUrl) ? parsed.apiUrl : undefined
-    return { ...parsed, apiUrl: envApi ?? persisted ?? DEFAULT_API }
+    if (envApi) return { ...parsed, apiUrl: envApi }
+    if (typeof parsed.apiUrl === 'string' && isRetired(parsed.apiUrl)) {
+      // Drop the stored session with the host: it was minted by a different deployment (see
</file context>

await (await load()).readGlobal()
expect(err).toHaveBeenCalledWith(expect.stringContaining(RETIRED))
expect(out).not.toHaveBeenCalled()
err.mockRestore(); out.mockRestore()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: A failed assertion in the stderr/stdout test leaves both process stream spies installed because cleanup is performed only after the assertions. Restoring the spies in a finally block or from afterEach would keep one test failure from contaminating later tests.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/config-api-url.test.ts, line 58:

<comment>A failed assertion in the stderr/stdout test leaves both process stream spies installed because cleanup is performed only after the assertions. Restoring the spies in a `finally` block or from `afterEach` would keep one test failure from contaminating later tests.</comment>

<file context>
@@ -35,11 +35,32 @@ test('a persisted retired host is replaced by the current default', async () =>
+  await (await load()).readGlobal()
+  expect(err).toHaveBeenCalledWith(expect.stringContaining(RETIRED))
+  expect(out).not.toHaveBeenCalled()
+  err.mockRestore(); out.mockRestore()
+})
+
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants